Skip to content

sqlite: avoid extra copy for large text binds #61580

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
thisalihassan wants to merge 1 commit into
base: main
Choose a base branch
from
Open

sqlite: avoid extra copy for large text binds #61580

thisalihassan wants to merge 1 commit into from
+61 −8

Conversation

@thisalihassan
Copy link
Contributor

@thisalihassan thisalihassan commented Jan 29, 2026 •
edited
Loading

Summary

  • When binding UTF-8 strings to SQLite statements, transfer ownership of the malloc-backed Utf8Value buffer to SQLite to avoid an extra copy for larger strings.
  • Use sqlite3_bind_blob64() for BLOB parameters to avoid truncation for larger payloads.

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jan 29, 2026

Review requested:

  • @nodejs/sqlite

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. sqlite Issues and PRs related to the SQLite subsystem. labels Jan 29, 2026
@thisalihassan thisalihassan force-pushed the sqlite-bind-text64-avoid-copy branch from ea75fcc to b7abb6f Compare January 29, 2026 21:00
@thisalihassan
Copy link
Contributor Author

thisalihassan commented Jan 29, 2026

Benchmarks (local, macOS, Release build)

Compared with base branch (main)

Summary:

  • INSERT INTO large_text (text_8kb_column) VALUES (?): *** +5.30% (±1.42%)
  • Other insert cases: within ~±1% (not significant), except all_column_types -0.77% (*) (±0.77%)

change primarily improves large text binds; other workloads appear neutral within measurement noise.

@codecov
Copy link

codecov bot commented Jan 29, 2026 •
edited
Loading

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 89.77%. Comparing base (37e4004) to head (da58cfd).
⚠️ Report is 27 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main   #61580      +/-   ##
==========================================
- Coverage   89.78%   89.77%   -0.01%     
==========================================
  Files         673      673              
  Lines      203775   203852      +77     
  Branches    39166    39181      +15     
==========================================
+ Hits       182956   183017      +61     
- Misses      13139    13153      +14     
- Partials     7680     7682       +2
Files with missing lines Coverage Δ
src/node_sqlite.cc 80.75% <100.00%> (+0.09%) ⬆️

... and 45 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@thisalihassan thisalihassan force-pushed the sqlite-bind-text64-avoid-copy branch 2 times, most recently from 92bb01c to 8e4016b Compare January 30, 2026 09:23
@thisalihassan
sqlite: avoid extra copy for large text binds
da58cfd 
When binding UTF-8 strings to prepared statements, transfer ownership of
malloc-backed Utf8Value buffers to SQLite to avoid an extra copy for
large strings. Use sqlite3_bind_blob64() when binding BLOB parameters.
@thisalihassan thisalihassan force-pushed the sqlite-bind-text64-avoid-copy branch from 8e4016b to da58cfd Compare January 30, 2026 09:30
@thisalihassan
Copy link
Contributor Author

thisalihassan commented Jan 30, 2026

Hi @cjihrig, @Renegade334 and @mcollina When you have a moment, could you please take a look at this PR and share your feedback? Thanks so much!

mcollina
mcollina approved these changes Jan 31, 2026
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@mcollina mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 31, 2026
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 31, 2026
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jan 31, 2026

CI: https://ci.nodejs.org/job/node-test-pull-request/71137/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mcollina mcollina mcollina approved these changes

Assignees

No one assigned

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. sqlite Issues and PRs related to the SQLite subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@thisalihassan @nodejs-github-bot @mcollina